Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change parser to parse lowercase forms of uppercase words separately #54

Merged
merged 5 commits into from
May 15, 2024

Conversation

jtmaxwell3
Copy link
Collaborator

@jtmaxwell3 jtmaxwell3 commented May 10, 2024

  • Change the parser to always parse a lowercase form of an uppercase wordform even if the uppercase wordform has a parse
  • Change the parser to store the lowercase analysis of an uppercase wordform on the lowercase wordform
  • Change RecordList to update Word Analyses window if a WfiWordformTags.kflidAnalyses change
  • Change IText.PropChanged to update uppercase wordforms if their lowercase forms change

This change is Reviewable

@jasonleenaylor
Copy link
Contributor

Src/LexText/Interlinear/InterlinDocRootSiteBase.cs line 1072 at r1 (raw file):

							}
						}
					}

I was looking at this code and considering if a more compact solution was possible.
I don't particularly like variables with a number on the end, they don't make for easy code reading.
Linq doesn't always make for easier reading either, but it is a bit less code.
Getting the lower case versions into the wordformsToUpdate could be done similar to the following (I haven't checked if this compiles).

  wf => {
    var otherForm = wf.Form.VernacularDefaultWritingSystem;
    return otherForm != null && form != otherForm && !string.IsNullOrEmpty(wf.Text) && cf.ToLower(otherForm.Text) == form.Text;
}

But that still leaves the issue of knowing if we should add a call to the idlequeue.
I guess we could check if we have changed the wordformsToUpdate count by checking the size at the beginning and end of the case statement...
What do you think?

Code quote:

					foreach (IWfiWordform wordform2 in uniqueWordforms)
					{
						var form2 = wordform2.Form.VernacularDefaultWritingSystem;
						if (form2 != form && form2 != null && !string.IsNullOrEmpty(form2.Text))
						{
							string sLower = cf.ToLower(form2.Text);
							if (sLower == form.Text)
							{
								m_wordformsToUpdate.Add(wordform2);
								m_mediator.IdleQueue.Add(IdleQueuePriority.High, PostponedUpdateWordforms);
							}
						}
					}

@jtmaxwell3
Copy link
Collaborator Author

I don't know anything about Linq. There could be more than one uppercase wordform (e.g. titlecase, uppercase, camelcase). I would be happy to rename wordform2 to ucWordform and form2 to ucForm to make the code clearer.

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, all discussions resolved


Src/LexText/Interlinear/InterlinDocRootSiteBase.cs line 1072 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

I was looking at this code and considering if a more compact solution was possible.
I don't particularly like variables with a number on the end, they don't make for easy code reading.
Linq doesn't always make for easier reading either, but it is a bit less code.
Getting the lower case versions into the wordformsToUpdate could be done similar to the following (I haven't checked if this compiles).

  wf => {
    var otherForm = wf.Form.VernacularDefaultWritingSystem;
    return otherForm != null && form != otherForm && !string.IsNullOrEmpty(wf.Text) && cf.ToLower(otherForm.Text) == form.Text;
}

But that still leaves the issue of knowing if we should add a call to the idlequeue.
I guess we could check if we have changed the wordformsToUpdate count by checking the size at the beginning and end of the case statement...
What do you think?

I think your suggested variable name updates are adequate.

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)

@jasonleenaylor jasonleenaylor merged commit e0fdca0 into release/9.1 May 15, 2024
5 checks passed
@jasonleenaylor jasonleenaylor deleted the LT-21425 branch May 15, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants